Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Create 0 size byte slices on allownil []byte elements #363

Merged
merged 3 commits into from
Sep 25, 2024
Merged

Create 0 size byte slices on allownil []byte elements #363

merged 3 commits into from
Sep 25, 2024

Conversation

klauspost
Copy link
Collaborator

@klauspost klauspost commented Sep 18, 2024 •
edited
Loading

When allownil is specified on []byte elements, create 0 byte slices when a 0 sized binary blob is read.

This makes []byte fully symmetric for roundtrips.

(Tests regression and also that fields are nilled correctly)

Fixes #362

Decoder:

		case "abyteslice":
			if msgp.IsNil(bts) {
				bts = bts[1:]
				z.AByteSlice = nil
			} else {
				z.AByteSlice, bts, err = msgp.ReadBytesBytes(bts, z.AByteSlice)
				if err != nil {
					err = msgp.WrapError(err, "AByteSlice")
					return
				}
				if z.AByteSlice == nil {
					z.AByteSlice = make([]byte, 0)
				}
			}

Unmarshal:

		case "abyteslice":
			if msgp.IsNil(bts) {
				bts = bts[1:]
				z.AByteSlice = nil
			} else {
				z.AByteSlice, bts, err = msgp.ReadBytesBytes(bts, z.AByteSlice)
				if err != nil {
					err = msgp.WrapError(err, "AByteSlice")
					return
				}
				if z.AByteSlice == nil {
					z.AByteSlice = make([]byte, 0)
				}
			}

@klauspost
Create 0 size byte slices
dbe715a 
As with other types, create a 0 size byte slice when the serialized data says so.

This is a functional change, and will require the use of `allownil` to retain the existing behaviour (ie no allocs on nil/zero length slices).

Fixes tinylib#362
@klauspost klauspost requested a review from philhofer September 18, 2024 08:29
@philhofer
Copy link
Member

Adopting this would create some odd asymmetry with the default behavior on the encoder side of things which always encodes slices as zero-length arrays, even if they are nil.

IMO if we are going to introduce a behavioral change, we should do a major version bump and clean up all of that stuff at once.

@klauspost
Copy link
Collaborator Author

@philhofer Yeah, I see your point. Maybe we should put it on the allownil generation, so it will apply only to those? That would AFAICT make it fully symmetric.

Let me see if that is feasible.

@klauspost klauspost changed the title Create 0 size byte slices Create 0 size byte slices on allownil []byte elements Sep 24, 2024
@klauspost
Copy link
Collaborator Author

@philhofer Description updated. General change reverted and only allownil functionality has changed. PTAL when possible.

@klauspost klauspost merged commit 3dc88ae into tinylib:master Sep 25, 2024
4 checks passed
@klauspost klauspost deleted the make-zero-byte-slices branch September 25, 2024 07:33
klauspost added a commit to klauspost/msgp that referenced this pull request Oct 22, 2024
@klauspost
Fix allownil propagated to byte slices as slice elements
483195a 
`[][]byte` would assume that the byte slices had allownil set. This would cause an extra `if x == nil {x = make([]byte,0)}` to be inserted on elements.

Regression from tinylib#363
@klauspost klauspost mentioned this pull request Oct 22, 2024
Merged
klauspost added a commit that referenced this pull request Oct 22, 2024
@klauspost
Fix allownil propagated to byte slices as slice elements (#374)
65798b6 
`[][]byte` would assume that the byte slices had allownil set. This would cause an extra `if x == nil {x = make([]byte,0)}` to be inserted on elements.

Regression from #363
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philhofer philhofer philhofer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allownil in []byte is useless
2 participants
@klauspost @philhofer